fix(repo-review): restart repo analysis on head drift - #3396

Walkthrough

The repository review coordinator now detects origin/main drift during later rounds, stops stale body-writer repairs, quarantines affected outputs, and restarts the repository workflow from round 1 within the existing repair budget. Documentation and tests cover the new behavior.

Changes

Source-head recovery

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: 🟡 Moderate · up to 4b7ae

The drift recovery improves source consistency, but a filesystem error during quarantine can still abort the cycle before stale outputs are made unavailable or failure is recorded. This fail-closed path should be fixed and tested before merge; restart timing and recovery documentation also need clarification.

Sequence Diagram(s)

sequenceDiagram
  participant Coordinator
  participant BodyWriter
  participant RepairDirectory
  participant RepositoryWorkflow
  Coordinator->>RepositoryWorkflow: run repository review
  RepositoryWorkflow->>BodyWriter: write review body
  BodyWriter-->>Coordinator: report source-head drift
  Coordinator->>BodyWriter: stop stale retries
  Coordinator->>RepairDirectory: quarantine prior round artifacts
  Coordinator->>RepositoryWorkflow: restart from round 1
  RepositoryWorkflow-->>Coordinator: return exact-head results

❌ Failed checks (1 warning)

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

openai

• Model: gpt-5.6-terra
• Verdict: PASS
• Confidence: 87%
• Scores:
  • Correctness: 9.0/10
  • Completeness: 9.0/10
  • Quality: 9.0/10
  • Testing: 9.0/10
  • Risks: 8.0/10
• Summary: The change implements bounded full-repository recovery when a failed later phase reports exact-head or source-provenance drift. It detects recognized drift diagnostics, prevents futile phase-local retries, quarantines repo-scoped stale analysis and logs, refreshes the active-repo preflight, and restarts processing from round 1 so checkout synchronization, GitNexus data, findings, negotiation, and body generation share fresh head provenance. Restart evidence and restart limits are recorded and use the existing repair-attempt budget. Documentation and configuration text were updated to describe the behavior and increased bounded runtime. The coordinator tests were substantially expanded to cover drift detection, quarantine/restart behavior, retry suppression, evidence, and bounded restart handling. The implementation is readable and appropriately isolates drift detection and restart preparation; the remaining reliance on known diagnostic text markers is a reasonable compatibility tradeoff given the existing subprocess/log-based phase interface.

anthropic

• Model: claude-sonnet-5
• Verdict: PASS
• Confidence: 72%
• Scores:
  • Correctness: 7.0/10
  • Completeness: 7.0/10
  • Quality: 7.0/10
  • Testing: 7.0/10
  • Risks: 6.0/10
• Summary: The PR implements head-drift detection and a bounded full-repo restart mechanism in the repo review coordinator, replacing futile per-phase retries when origin/main advances mid-run. Config and docs are updated consistently to describe the new restart behavior and its bounded budget reuse. The code adds a stop_retry_when hook to the existing repair loop, a marker-based drift detector, and a prepare_head_drift_restart function that quarantines round-1/round-2/body-writer artifacts and logs before restarting at round 1. Test file additions (331 lines) appear to substantially cover the new logic, and CI (selftest-ci.yml, pr-11-ci-smoke.yml) both report success on relevant test suites, supporting the acceptance criteria's pytest requirements. The diff was truncated before showing the full implementation and its integration into the main loop, limiting full correctness verification, but the visible changes are coherent, well-documented, and consistent with the stated acceptance criteria. Given the additive nature of the change (new optional parameter, new functions) and passing CI test suites, this satisfies the acceptance criteria with only minor completeness/robustness concerns around the marker-based drift heuristic and unseen loop integration details.
• Concerns:
  • The diff was truncated in review; the full implementation of prepare_head_drift_restart and its integration into the main coordination loop could not be fully verified for correctness (e.g., whether restart_number/repair budget accounting correctly bounds restarts, and whether quarantine correctly moves all round-1/round-2/body-writer artifacts).
  • Head drift detection relies on substring matching against a fixed tuple of log message markers (HEAD_DRIFT_MARKERS); this is brittle if underlying error messages change wording, potentially causing false negatives (missed drift detection) or false positives.
  • Cannot independently confirm the stated test counts (24 passed / 446 passed) from the diff alone, though CI results (selftest-ci.yml Python Tests success) support tests are passing.
  • Documentation changes (config toml and REPO_REVIEW_PROCESS.md) appear consistent with the described behavior, but full verification of interaction between stop_retry_when callback and existing repair loop logic requires seeing the complete run_subprocess_with_repairs and caller code, which was cut off.
  • New Callable-based stop_retry_when parameter changes the signature of run_subprocess_with_repairs; if there are other callers not shown in the diff, backward compatibility should be confirmed (though default None suggests it's additive and safe).